MTHINC#1303
Conversation
Claude Code ReviewHead SHA: 202d7da Summary
Findings[HIGH] Missing Fortran-side runtime validation for [HIGH] [MEDIUM] Backward compatibility break: [MEDIUM] Removed constraint on [MEDIUM] [LOW] Hardcoded [LOW] Redundant precision cast [LOW] MTHINC not actually exercised in some new tests |
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd MTHINC interface compression and refactor THINC functionality
WalkthroughsDescription• Implements MTHINC (multi-dimensional THINC) interface compression for volume fraction sharpening alongside existing THINC support • Introduces new m_thinc module with helper functions for stable logarithmic-cosine computation, 1D THINC integrals, and multi-dimensional volume integrals using Gaussian quadrature • Converts int_comp parameter from logical to integer type with three states: 0=off, 1=THINC, 2=MTHINC • Integrates THINC/MTHINC compression into RHS computation with Newton iteration for interface position parameter solving • Refactors interface compression functionality out of MUSCL module into dedicated m_thinc module • Updates MPI communication to handle new integer parameter type • Adds comprehensive test cases for MTHINC in 2D/3D configurations with WENO reconstruction • Updates all example cases and parameter definitions to use new integer enum values • Includes performance validation showing ~3% overhead for MTHINC vs ~1% for THINC • Updates documentation and validation logic for the new parameter type and options Diagramflowchart LR
A["int_comp: logical<br/>to integer"] --> B["m_thinc module<br/>THINC + MTHINC"]
B --> C["RHS computation<br/>with compression"]
B --> D["Startup initialization<br/>and finalization"]
C --> E["Test cases<br/>2D/3D MTHINC"]
A --> F["MPI broadcast<br/>parameter update"]
A --> G["Parameter validation<br/>0/1/2 values"]
File Changes1. src/simulation/m_thinc.fpp
|
Code Review by Qodo
1. Compression skipped on split recon
|
Claude Code ReviewHead SHA: Key source files:
Summary
Findings[Medium] Missing Fortran runtime check for
@:PROHIBIT(int_comp < 0 .or. int_comp > 2, "int_comp must be 0 (off), 1 (THINC), or 2 (MTHINC)")The Python validator catches this at case-setup time, but the Fortran check is the convention for parameters with constrained integer values. [Medium] MTHINC silently degrades to 1-D THINC for 1-D cases — When
Mathematically this is fine (MTHINC reduces to THINC in 1-D), but the user gets THINC behaviour when requesting MTHINC with no diagnostic. The old Python constraint that required [Low] MTHINC skips monotonicity check — The 1-D THINC path guards with [Low] Two-fluid assumption is undocumented — Both THINC and MTHINC update only [Positive] Correct THINC bug fix The old right-reconstruction in vR...(contxb) = vL...(contxb) / vL...(advxb) * aTHINC ! Bug: advxb already overwrittenThe new code in [Positive] Architecture improvement Centralising interface compression into |
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated THINC/MTHINC interface-compression module and migrates the int_comp setting from a logical flag to an integer selector, while updating tests/docs/tooling accordingly.
Changes:
- Added
src/simulation/m_thinc.fppimplementing THINC (int_comp=1) and MTHINC (int_comp=2) and wired it into the reconstruction flow inm_rhs. - Replaced the old MUSCL/WENO-local interface-compression hooks by centralizing compression in RHS reconstruction; removed the MUSCL
s_interface_compressionimplementation. - Updated toolchain parameter typing/validation and adjusted examples/docs/tests to use integer
int_comp.
Reviewed changes
Copilot reviewed 42 out of 81 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/simulation/m_thinc.fpp |
New THINC/MTHINC implementation (including MTHINC normal computation and Newton solve). |
src/simulation/m_rhs.fpp |
Calls MTHINC normal computation pre-split; runs compression post-reconstruction with NVTX ranges. |
src/simulation/m_muscl.fpp |
Removes s_interface_compression and MUSCL-side compression call. |
src/simulation/m_weno.fpp |
Removes MUSCL import and WENO-side compression call. |
src/simulation/m_start_up.fpp |
Initializes/finalizes the new THINC module when int_comp > 0. |
src/simulation/m_global_parameters.fpp |
Changes int_comp from logical to integer with updated defaults/comments. |
src/simulation/m_mpi_proxy.fpp |
Broadcasts int_comp as an integer instead of logical. |
toolchain/mfc/params/definitions.py |
Registers int_comp as INT instead of LOG. |
toolchain/mfc/case_validator.py |
Validates int_comp is in {0,1,2}. |
toolchain/mfc/test/cases.py |
Expands generated test matrix to include int_comp values {0,1,2}. |
toolchain/mfc/params/descriptions.py |
Updates parameter description to document 0/1/2 meanings. |
docs/documentation/case.md |
Updates docs to describe integer int_comp and mentions MTHINC. |
docs/module_categories.json |
Adds m_thinc to documentation module grouping. |
examples/*/case.py |
Updates examples from "T" to 1 for THINC. |
tests/*/golden-metadata.txt |
Updates/adds golden metadata for the new/updated test set. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis pull request refactors interface compression from a boolean flag into an integer enumeration with three states: 0 (off), 1 (THINC), and 2 (MTHINC). The implementation moves compression logic from the MUSCL module into a dedicated new THINC module. Interface compression is now integrated into RHS calculations and invoked during variable reconstruction. Documentation, parameter definitions, validation logic, and example configurations are updated to reflect the new integer-based parameter type and semantic values. Test cases are expanded to exercise the new enumeration states. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 411ddfaf-e410-411e-a688-203d6a1caf71
📒 Files selected for processing (81)
docs/documentation/case.mddocs/module_categories.jsonexamples/1D_sodshocktube_muscl/case.pyexamples/2D_advection_muscl/case.pyexamples/2D_riemann_test_muscl/case.pyexamples/2D_shockdroplet_muscl/case.pyexamples/3D_rayleigh_taylor_muscl/case.pyexamples/3D_shockdroplet_muscl/case.pysrc/simulation/m_global_parameters.fppsrc/simulation/m_mpi_proxy.fppsrc/simulation/m_muscl.fppsrc/simulation/m_rhs.fppsrc/simulation/m_start_up.fppsrc/simulation/m_thinc.fppsrc/simulation/m_weno.fpptests/09195EF4/golden-metadata.txttests/09195EF4/golden.txttests/0A362971/golden-metadata.txttests/0A362971/golden.txttests/11D609F6/golden-metadata.txttests/11D609F6/golden.txttests/1B300F28/golden-metadata.txttests/1F0C4A44/golden-metadata.txttests/1F0C4A44/golden.txttests/2C9844EF/golden-metadata.txttests/2C9844EF/golden.txttests/2D11A034/golden-metadata.txttests/2D11A034/golden.txttests/2E4AC44C/golden-metadata.txttests/2E4AC44C/golden.txttests/3ECA875A/golden-metadata.txttests/3ECA875A/golden.txttests/409B55DF/golden-metadata.txttests/409B55DF/golden.txttests/503EEFF7/golden-metadata.txttests/503EEFF7/golden.txttests/50EC2239/golden-metadata.txttests/50EC2239/golden.txttests/6102D509/golden-metadata.txttests/6102D509/golden.txttests/6775D763/golden-metadata.txttests/6775D763/golden.txttests/67C777D8/golden.txttests/7156050E/golden-metadata.txttests/7156050E/golden.txttests/845DC70C/golden-metadata.txttests/845DC70C/golden.txttests/885D5D8C/golden-metadata.txttests/885D5D8C/golden.txttests/8876692F/golden-metadata.txttests/8876692F/golden.txttests/8C42A56C/golden-metadata.txttests/8C42A56C/golden.txttests/A438B8CE/golden-metadata.txttests/A438B8CE/golden.txttests/A930AE61/golden-metadata.txttests/A930AE61/golden.txttests/AEE7A791/golden-metadata.txttests/AEE7A791/golden.txttests/B3C724B5/golden-metadata.txttests/B3C724B5/golden.txttests/B903D17E/golden-metadata.txttests/B903D17E/golden.txttests/C02D71EE/golden-metadata.txttests/C02D71EE/golden.txttests/C420EDF3/golden-metadata.txttests/C420EDF3/golden.txttests/CE35B602/golden-metadata.txttests/CE35B602/golden.txttests/E11FD23A/golden-metadata.txttests/E11FD23A/golden.txttests/F5FABAE9/golden-metadata.txttests/F5FABAE9/golden.txttests/FA4D8FEF/golden-metadata.txttests/FA4D8FEF/golden.txttests/FE9379D8/golden-metadata.txttests/FE9379D8/golden.txttoolchain/mfc/case_validator.pytoolchain/mfc/params/definitions.pytoolchain/mfc/params/descriptions.pytoolchain/mfc/test/cases.py
💤 Files with no reviewable changes (10)
- tests/FA4D8FEF/golden.txt
- tests/FA4D8FEF/golden-metadata.txt
- tests/2C9844EF/golden-metadata.txt
- src/simulation/m_weno.fpp
- tests/0A362971/golden.txt
- tests/1B300F28/golden-metadata.txt
- tests/8876692F/golden-metadata.txt
- tests/C02D71EE/golden-metadata.txt
- tests/50EC2239/golden-metadata.txt
- tests/0A362971/golden-metadata.txt
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1303 +/- ##
==========================================
+ Coverage 64.75% 64.89% +0.13%
==========================================
Files 71 72 +1
Lines 18721 18878 +157
Branches 1551 1571 +20
==========================================
+ Hits 12123 12251 +128
- Misses 5640 5651 +11
- Partials 958 976 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Claude Code ReviewHead SHA: 85acf5a Files changed: 81 (key source files: Summary
Findings[Medium] if (int_comp == 2 .and. n > 0) then ! MTHINC guard
[Medium] Newton solver do iter = 1, 30
...
d = d - residual/dV
end doIf convergence fails (e.g., degenerate normal near a triple point or machine-epsilon gradient), the function returns an unclamped [Low] Removed [Low] Unused allocations for MTHINC in 1D [Info] Bug fix for #1181 confirmed correct ! BUG: vL_rs_vf_XYZ(advxb) already overwritten by left reconstruction above
vR_rs_vf_XYZ(contxb) = vL_rs_vf_XYZ(contxb) / vL_rs_vf_XYZ(advxb) * aTHINCNew code correctly saves Overall the implementation is well-structured. The |
sbryngelson
force-pushed
the
MTHINC
branch
2 times, most recently
from
2e0268f to
617f985
Compare
Claude Code ReviewIncremental review from: (first review) New findings:1. Critical — GPU coherence: All three variables are read inside
By contrast, the analogous scalar Fix: Add to $:GPU_DECLARE(create='[int_comp,ic_eps,ic_beta]')And add to $:GPU_UPDATE(device='[int_comp,ic_eps,ic_beta]')2. Moderate — MTHINC density reconstruction silently skips intermediate fluid components for In rho1 = v_vf(contxb)%sf(ix, iy, iz)/aC
rho2 = v_vf(contxe)%sf(ix, iy, iz)/(1._wp - aC)This implicitly assumes exactly 2 fluids. For There is no constraint in Fix: Either extend the density reconstruction to loop over all fluids from self.prohibit(int_comp > 0 and num_fluids > 2,
"int_comp (THINC/MTHINC) only supports num_fluids <= 2") |
|
<!-- claude-review: thread=primary; reviewed_sha=0ed0772ee8995e5752187f6d67d035a6bc49b556; mode=incremental --> |
|
@wilfonba can you re-run a few sanity checks to make sure this works after i made a bunch of changes? then i guess we can mark it ready to review, which ensures it works on frontier/phoenix/whatever. then merge. |
Yep. I'll send you a message and open it as ready to review when I've taken care of that. |
Claude Code ReviewHead SHA: c183e43 Files changed:
FindingsGPU device data not initialized for MTHINC quadrature constants (
|
sbryngelson
force-pushed
the
MTHINC
branch
4 times, most recently
from
416d0ad to
755fba3
Compare
|
@sbryngelson, I believe this is ready to merge. The failed benchmark shows good performance if you look at the generated comment. There was just some sort of error with the Slurm job. I can rerun it still if you want though. |
Code reviewFound 2 issues:
MFC/toolchain/mfc/case_validator.py Lines 369 to 382 in 9c875b1
MFC/src/simulation/m_start_up.fpp Lines 914 to 926 in 9c875b1 MFC/src/simulation/m_start_up.fpp Lines 1085 to 1096 in 9c875b1 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
|
This was a good catch by Claude. A bunch of the tests for M/THINC were one fluid. I'll work on removing the existing garbage tests and creating new ones |
- case.md: add muscl_eps row/description alongside int_comp (MTHINC version) - m_global_parameters.fpp: keep int_comp as integer (0/1/2), add muscl_eps; keep GPU_DECLARE; both GPU_UPDATEs - m_mpi_proxy.fpp: broadcast both int_comp and collision_model as integers - case_validator.py: keep recon_type validation from master; drop stale int_comp=boolean check (superseded by check_interface_compression)
sbryngelson
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
Code looks good. Both issues from the April 5 review are fixed (num_fluids constraint and init/finalize guard). Merge conflicts resolved.
|
Persistent review updated to latest commit c183e43 |
| ac = v_vf(eqn_idx%adv%beg)%sf(j, k, l) | ||
|
|
||
| if (ac >= ic_eps .and. ac <= 1._wp - ic_eps) then | ||
| nr_x = (v_vf(eqn_idx%adv%beg)%sf(j + 1, k, l) - v_vf(eqn_idx%adv%beg)%sf(j - 1, k, l))*5e-1_wp | ||
|
|
||
| nr_y = 0._wp | ||
| if (n > 0) then | ||
| nr_y = (v_vf(eqn_idx%adv%beg)%sf(j, k + 1, l) - v_vf(eqn_idx%adv%beg)%sf(j, k - 1, l))*5e-1_wp | ||
| end if | ||
|
|
||
| nr_z = 0._wp | ||
| if (p > 0) then | ||
| nr_z = (v_vf(eqn_idx%adv%beg)%sf(j, k, l + 1) - v_vf(eqn_idx%adv%beg)%sf(j, k, l - 1))*5e-1_wp | ||
| end if |
There was a problem hiding this comment.
1. Mthinc normals ignore spacing 🐞 Bug ≡ Correctness
s_compute_mthinc_normals computes the interface normal using unscaled index-space differences, so on nonuniform/stretched grids the normal direction is wrong and MTHINC face averages are computed for the wrong orientation. This can silently distort interface sharpening and fluxes in real runs without crashing.
Agent Prompt
### Issue description
`src/simulation/m_thinc.fpp:s_compute_mthinc_normals` computes MTHINC interface normals using raw index-space differences (no dx/dy/dz or coordinate-distance scaling). On nonuniform grids this produces incorrect normal directions and therefore incorrect MTHINC face-averaged volume fractions.
### Issue Context
Other gradient computations in the simulation divide by `dx`, `dy`, `dz`, so the codebase expects physical-space gradients.
### Fix Focus Areas
- src/simulation/m_thinc.fpp[246-283]
### What to change
- Replace the current centered differences with metric-scaled differences, e.g.:
- `nr_x = (alpha(j+1)-alpha(j-1)) / (x_cc(j+1)-x_cc(j-1))` (or `/ (dx(j)+dx(j-1))` depending on grid definitions)
- same for y/z using `y_cc`/`dy`, `z_cc`/`dz`.
- Keep the normalization to unit length afterward.
- Ensure the chosen metric works for cylindrical/axisymmetric setups if supported by the rest of the solver.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Persistent review updated to latest commit c183e43 |
Claude Code ReviewPR #1303 — MTHINC This PR adds MTHINC interface compression and refactors THINC into Critical Issues1. GPU device memory never initialized for Gauss-quadrature constants — MTHINC produces garbage on GPU real(wp) :: gq3_pts(3) = [...]
real(wp) :: gq3_wts(3) = [...]
real(wp) :: ln2 = 0.6931471805599453_wp
$:GPU_DECLARE(create='[gq3_pts, gq3_wts, ln2]')
These constants are read inside GPU The codebase pattern is clear: subroutine s_initialize_thinc_module()
$:GPU_UPDATE(device='[gq3_pts, gq3_wts, ln2]')
...
end subroutineImportant Issues2. MTHINC normals ignore grid spacing — incorrect on non-uniform and anisotropic grids nr_x = (v_vf(eqn_idx%adv%beg)%sf(j+1,k,l) - v_vf(eqn_idx%adv%beg)%sf(j-1,k,l))*5e-1_wp
nr_y = (v_vf(eqn_idx%adv%beg)%sf(j,k+1,l) - v_vf(eqn_idx%adv%beg)%sf(j,k-1,l))*5e-1_wpNo division by Fix: nr_x = (alpha_jp1 - alpha_jm1) / (x_cc(j+1) - x_cc(j-1))
nr_y = (alpha_kp1 - alpha_km1) / (y_cc(k+1) - y_cc(k-1))
nr_z = (alpha_lp1 - alpha_lm1) / (z_cc(l+1) - z_cc(l-1))3. 4-space indentation throughout Project standard enforced by Strengths
|
| if (int_comp > 0 .and. v_size >= eqn_idx%adv%end) then | ||
| call nvtxStartRange("WENO-INTCOMP") | ||
| #:for WENO_DIR, XYZ in [(1, 'x'), (2, 'y'), (3, 'z')] | ||
| if (weno_dir == ${WENO_DIR}$) then | ||
| call s_thinc_compression(v_rs_ws_${XYZ}$, vL_rs_vf_x, vL_rs_vf_y, vL_rs_vf_z, vR_rs_vf_x, vR_rs_vf_y, & | ||
| & vR_rs_vf_z, weno_dir, is1_weno, is2_weno, is3_weno) | ||
| end if | ||
| #:endfor | ||
| call nvtxEndRange() | ||
| end if |
There was a problem hiding this comment.
1. Compression skipped on split recon 🐞 Bug ≡ Correctness
s_thinc_compression indexes reconstruction arrays using global eqn_idx%cont/%adv indices, but reconstruction is frequently invoked on reindexed slices of the variable vector; the new guard v_size >= eqn_idx%adv%end causes interface compression to be silently skipped in common split-reconstruction paths (e.g., viscous/surface-tension branches), meaning int_comp has no effect in those runs.
Agent Prompt
### Issue description
Interface compression is called from WENO/MUSCL on the reconstruction-time variable slice, but `s_thinc_compression` uses global `eqn_idx` indices (cont/adv) to index into those slice-local arrays. The current workaround guard (`v_size >= eqn_idx%adv%end`) prevents out-of-bounds, but also disables interface compression in split reconstruction paths (e.g., viscous/surface-tension branches in `m_rhs`).
### Issue Context
`m_rhs` reconstructs different variable ranges in separate calls depending on physics settings; interface compression needs cont+adv together (and needs correct indexing) to function.
### Fix Focus Areas
- src/simulation/m_weno.fpp[1386-1395]
- src/simulation/m_muscl.fpp[213-221]
- src/simulation/m_rhs.fpp[657-696]
- src/simulation/m_rhs.fpp[1609-1648]
- src/simulation/m_thinc.fpp[294-411]
### Recommended fix options (pick one)
1) **Move compression to `m_rhs` (preferred for correctness):**
- Always reconstruct cont+adv in the same call when `int_comp>0` (even if other variables are reconstructed separately).
- Then call `s_thinc_compression` with full (global-indexed) arrays, avoiding slice reindexing entirely.
2) **Make `s_thinc_compression` slice-aware:**
- Pass an `iv_beg` (global offset) into WENO/MUSCL and then into `s_thinc_compression`.
- Compute local indices: `adv_local = eqn_idx%adv%beg - iv_beg + 1`, similarly for `cont_local`.
- Use these local indices for `v_rs_ws` and `vL/vR` access.
- Update the call guard to check presence via offsets rather than `v_size >= eqn_idx%adv%end`.
Add/extend a test where `Re_size != 0` (or surface tension path) and `int_comp=1/2` to ensure compression is actually applied (not silently skipped).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Description
This PR adds MTHINC to the code base and fixes the THINC bug in #1181. The existing golden files for THINC interface compression were preserved, and new golden files and tests were created for MTHINC.
Type of change
Testing
3D advection case with and without MTHINC.
test.mp4
2D advection with
int_comp = 1(THINC) 1 rank vs. 4 rank. Results are identicaltest.mp4
2D advection with
int_comp = 2(MTHINC) 1 rank vs. 4 rank. Results are identicaltest.mp4
3D advection with
int_comp = 2(MTHINC) 1 rank vs. 8 rank. Results are identicalI didn't bother making a video for this one, but the output was identical when compared in ParaView.
Performance tests in 3D
The following results are grind times for a 200^3 advection case run on 1 A100 with OpenACC. The overhead for MTHINC is ~3% while the overhead for THINC is ~1%. The grind times are:
(Updated 4/1/2026 after removing
parameterfrom constants inm_thinc.fppint_comp = 0: 0.87488 (No compression, baseline)int_comp = 1: 0.88406 (1%) (THINC compression)int_comp = 2: 0.90199 (3%) (MTHINC compression)--nsysresults withint_comp = 2This is the run time contribution for just the MTHINC routines.
Time (%) Total Time (ns) Instances Avg (ns) Med (ns) Min (ns) Max (ns) StdDev (ns) Name
Checklist
See the developer guide for full coding standards.
GPU changes (expand if you modified
src/simulation/)AI code reviews
Reviews are not triggered automatically. To request a review, comment on the PR:
@coderabbitai review— incremental review (new changes only)@coderabbitai full review— full review from scratch/review— Qodo review/improve— Qodo code suggestions